gh-144370: Disallow usage of control characters in status in Lib/wsgiref/handlers.py for security#144371
gh-144370: Disallow usage of control characters in status in Lib/wsgiref/handlers.py for security#144371benediktjohannes wants to merge 65 commits intopython:mainfrom
Lib/wsgiref/handlers.py for security#144371Conversation
|
Important note: This PR needs backport to 3.10, 3.11, 3.12, 3.13 and 3.14 please, thanks! 👍 (I'll probably add these tomorrow if there are no concerns.) |
|
@gpshead or @StanFromIreland / @ZeroIntensity please review, thanks! |
|
What is different from #144118? |
|
I would suggest that you directly comment on Seth's PR instead. |
|
@picnixz I guess that this was a misunderstanding, this PR is about a completely different thing that is changes. This is not a duplicate of the other PR because the other one is about adding HTAB again to Lib/wsgiref/headers.py, but this one is about Lib/wsgiref/handlers.py, not headers.py. This is something completely different because the other one only adds one character to be allowed, but this one reguards disallowing several characters in handlers.py (and not headers.py) which wasn‘t mentioned at all in the other PR because this concerns completely different aspects like status and some changes in debug mode in handlers.py and not headers.py. Please reopen. Thanks! |
|
@picnixz I guess, I know why this seemed to be a duplicate as I described the „differences“ to the other PR and then you thought that this was about some changes that I suggested for the other PR, but this is not what this PR is about because this is about something completely different in another location, so please reopen. Thanks! |
|
And I think that is would not be a very good idea if we combined these changes in another PR because the first one was already merged two weeks ago and the other one is only about adding allowed phrases again while this one is about the opposit (disallowing phrases) with the only thing in common that this one does not disallow one thing which will be added in the new PR again, but both other PRs concern another location which makes this not even a duplication in that way, so please reopen. Thanks! |
|
Thank you very much! |
vstinner
left a comment
vstinner
left a comment
There was a problem hiding this comment.
You should write tests for theses changes.
Co-authored-by: Victor Stinner <vstinner@python.org>
|
Hi @vstinner thank you very much for your review! I really like your suggestions and I'll make some commits in order to get them in the PR soon, thank you very much! |
Co-authored-by: Victor Stinner <vstinner@python.org>
|
(Added the "re" again because it's included in vstinner's regex and also in sethmlarson's PR so if this will be used in both cases then it's consistent in that way and it would take too much time to change this there as well and the suggestions by seth and vstinner were both with the "re", so I've added it) |
|
I'm not quite sure about the status whether it should be handled as a value or a name, so please correct me if I'm mistaken inthis case (or even none of both? but I don't think that) |
Tests are basically already included by seth's PR, but I can of course add similar ones to handlers as well. |
Please add new tests on handlers. |
|
@vstinner please re-review, thanks! (I'm still not quite sure about it because I haven't tested it (also the tests are not tested) or is this automatically done by the lint check?) |
|
(but even if this is done by the lint check automatically, I'm not quite sure about the handling of the status) |
|
and thanks for the review by the way 👍 |
|
Ahh, this seems actually to be checked automatically by the Tests based on what I've seen! Nice! But I'm not quite sure about the correct classification of the status nevertheless, so please have a look there, thanks! |
Co-authored-by: Victor Stinner <vstinner@python.org>
Co-authored-by: Victor Stinner <vstinner@python.org>
… also in this case
|
Thanks @vstinner for the suggestions! Please also have a look at all the messages above (if you haven't already, e.g. when it comes to HTAB (whether we should use the chars allowed in names or values)) and thank you for your reviews! I hope that everything is fine now! 👍 |
|
Should be good to Go now, please have a look on it (and also on the messages above because of HTAB and so on), @vstinner, thank you! |
Co-authored-by: Victor Stinner <vstinner@python.org>
Co-authored-by: Victor Stinner <vstinner@python.org>
|
Suggested changes look good to me, thanks and thank you for the label! 👍 Please also have a look at the handling of HTAB in status and the other messages above, thanks! |
benediktjohannes
changed the title
Lib/wsgiref/handlers.py for securityLib/wsgiref/handlers.py for security
vstinner
left a comment
vstinner
left a comment
There was a problem hiding this comment.
LGTM.
@sethmlarson: Would you mind to review this change?
Misc/NEWS.d/next/Security/2026-01-31-21-56-54.gh-issue-144370.fp9m8t.rst
Outdated
Show resolved
Hide resolved
…fp9m8t.rst Co-authored-by: Victor Stinner <vstinner@python.org>
|
Thanks! |
5 participants
This PR is opened as public as the primary described CVE is already publicated and this only contains one certain point that has to be changed in code in Python and refers to issue #144370 (gh-144370).
In Lib/wsgiref/handlers.py at 260 (def _convert_string_type(self, value, title):) it is necessary to add the same fix as in #143916 requested as well (credits to @sethmlarson 👍). This references #143917 and #144118 and already includes the latest fix for the inclusion of HTAB, additions to this in form of tests will automatically be added by merging #144118.
There are some differences to wsgiref.headers.Headers that I made because I chose the default status of "name" to be "True" instead of false as "True" includes one more disallowed character for safety in future use cases if not defined otherwise, but I also added "name=True" even if not necessary in use cases just for safety for another case (if the default case was changed in order to produce similarity to wsgiref.headers.Headers while I'd recommend to change wsgiref.headers.Headers). I've also made an addition to the raised "ValueError" including "values" (without a further description of the very exact in- and exclusions like HTAB because this doesn't seem to be user-sided relevant to me) and I've used a slightly shorter internal form for the variables (which can be changed if wanted, but it's just style).
Important note: I was / am not quite sure about these changes as they were not tested, but I'm highly confident that this should be correct as it (mostly) bases on changes which were already made in other PRs (or at least in one other PR because this is the only merged one (but this also refers to a "non-merged" PR, so has to be concretized in that way)). And another important aspect is that I was not quite sure about the "status" (whether my classification is correct here as a "non-name value" when considering the handling of the characters, so please correct me if I'm mistaken (and I wasn't quite sure as well at the question whether potentially there should be added some different disallowed characters for this or some disallowed be removed, so please correct me if I'm mistaken)). Please also correct me if I'm mistaken in general anywhere in this PR (maybe this isn't even necessary for this case and I'm completely mistaken because this wasn't tested, but I'm pretty confident just from looking at the code).